[megatron] feat: migrate fused logprob/entropy from GPTModel.forward monkey-patch to Megatron output_processor hook#6933
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an alternative hook-based path (output_processor) in verl/models/mcore/model_forward_fused.py for Megatron-Core, which avoids monkey-patching GPTModel.forward by passing a callback and context directly to the native forward method. The hook path is controlled via the VERL_FUSED_USE_OP_HOOK environment variable and includes capability checks. The feedback suggests removing a redundant assertion checking if weight is None in fused_output_processor, as the subsequent call to linear_cross_entropy would naturally fail with a clear error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Quick follow-up: besides the offline bit-identical A/B in the PR description, I ran a small live end-to-end smoke on Megatron-Core 0.18 to make sure the hook path survives the full RL loop, not just an isolated forward. Setup was a dense Qwen2.5-0.5B on 4×A100, megatron engine (v1) with TP=2/PP=1, Both paths completed all 5 steps and saved checkpoints cleanly, with no NaN/inf and comparable, sane metrics (entropy ~0.43–0.66, training log-ppl ~0.40–0.62 on both). The three integration points I was worried about all held up under the hook: rollout↔actor resharding (actor/rollout prob correlation ~0.9996 each step), offload interacting with the hook path, and dist-checkpoint save/load. I also added a one-off build-time check to confirm the hook is genuinely what's running: under One honest caveat: the per-step metrics differ slightly between the two runs, but that's rollout non-reproducibility (vLLM across independent processes samples different trajectories), not the hook — the two runs simply don't see the same data. The exact numerical equivalence is what the offline A/B in the PR description covers ( Heads-up on one prerequisite unrelated to this PR: bringing up the megatron backend on Megatron-Core 0.18 currently also needs |
…cessor hook Replace the full GPTModel.forward monkey-patch (which existed only to pass `temperature` into _postprocess) with Megatron PR verl-project#4686's `output_processor` hook (core >= 0.18). Gated behind VERL_FUSED_USE_OP_HOOK (default 0 = legacy path, byte-identical to today). - Add use_output_processor_hook(), FusedOutputProcessorContext, and the fused_output_processor callback (SP gather + linear_cross_entropy at the _postprocess boundary -> CausalLMOutputForPPO). - patch_fused_forward / unpatch_fused_forward: no-op in hook mode. - fused_forward_model (legacy) and fused_forward_model_engine (live engine path): pass temperature via output_processor_context instead of temperature=. - Fail fast at build time via signature introspection when hook mode is requested but GPTModel.forward lacks `output_processor`. Downstream A/B (hook vs patch) is bit-identical (log_probs/entropy max_abs=0) across TP=1/2, tied/non-tied, SP gather, real THD training-shape batches, and the engine live path; forward memory delta is 0. Refs NVIDIA/Megatron-LM#4590.
Assisted-by: OpenAI Codex Signed-off-by: chengcuiping <96756894+chengcuiping@users.noreply.github.com>
Assisted-by: OpenAI Codex Signed-off-by: chengcuiping <96756894+chengcuiping@users.noreply.github.com>
d330a7f to
f49866a
Compare
What does this PR do?
verl/models/mcore/model_forward_fused.pycurrently replaces the fullGPTModel.forwardthrough_fused_GPTModel_forwardso the training loop can passtemperatureinto Megatron's_postprocessboundary, wherelinear_cross_entropycomputes fused log-probabilities and entropy.Megatron-LM PR #4686 added the native
output_processor/output_processor_contextextension point at that boundary. This PR uses that hook for verl's fused Megatron log-probability and entropy path while retaining the legacy monkey-patch as a compatibility fallback.The fused forward mode is decided once at model build time:
autois the default. It uses the native hook when the actual bound Megatron modelforwardsupports bothoutput_processorandoutput_processor_context, and otherwise falls back to the legacy patch.hookrequires the native hook and fails fast during model construction if the required contract is unavailable.legacyforces the existing monkey-patchedGPTModel.forwardpath.The selected mode is stored on the real Megatron model instance. Runtime forward calls read that fixed mode and do not repeatedly inspect the environment.
The live Megatron engine path is covered through
fused_forward_model_engine. The legacyfused_forward_modelcaller is kept compatible as well. Megatron'sbuild_schedule_plan()/ 1F1B postprocess integration is outside this PR and remains part of the broader upstream work tracked in NVIDIA/Megatron-LM#4590.Checklist Before Starting
[module] type: descriptionformat.Test
Focused tests added in this revision
Result:
The focused tests cover:
autoselecting the hook path when the bound model forward supports the full hook contract.autofalling back to the legacy monkey-patch when the hook contract is unavailable.hookmode failing fast during model construction when required parameters are missing.ValueError.temperaturethroughoutput_processor_contextwithout forwardingtemperature=to native Megatronforward.temperature=to the patched forward.Pre-submit checks:
Result:
Result:
Previously completed validation
The numerical and live integration validation below was completed before the latest rebase and was not rerun in full for this revision.
Environment used for the same-batch numerical validation:
log_probs/entropymax_abs=0padding_causal, GRPO advantages, including the coveredpg_losspathmax_abs=0fused_forward_model_enginewith build-time patch selectionlog_probs/entropymax_abs=0A separate 4×A100 five-step GRPO smoke previously exercised:
Both the hook and legacy paths completed that five-step integration run. The two independent runs saw different sampled trajectories, so per-step metrics were not expected to be identical; exact numerical equivalence was established by the same-weight, same-batch A/B validation above.
Forward memory usage was also measured as identical between the hook and legacy paths (
Δ = 0).Rebased local smoke limitation
A rebased local smoke was attempted, but the checkout runtime did not match the project's full CUDA image dependencies and the run stopped before Megatron model construction. The
output_processorpath was therefore not reached in that local run.The focused tests above pass, and the previously completed same-batch numerical validation and live five-step integration validation remain passing. The official project CI environment will provide the final rebased integration check.
Known limitation
The FP8 padding path was not exercised:
All numerical validation reported above used bf16. The callback itself does not introduce FP8-specific logic, but this PR does not claim coverage of the FP8 input-padding path.
API and Usage Example
Leaving
VERL_FUSED_USE_OP_HOOKunset or setting it to an empty value is equivalent toauto.Accepted aliases are:
Any other value raises
ValueErrorduring model construction.Design & Code Changes
VERL_FUSED_USE_OP_HOOKonce duringpatch_fused_forward.model.forwardsignature for bothoutput_processorandoutput_processor_context.GPTModel.forwarduntouched and do not createforward_backup.forward_backupcompatibility path.RuntimeErrorwhen forced hook mode is requested but the bound forward lacks the required hook contract.Two correctness details are intentional:
Megatron passes
output_processor_contextto the callback using the keywordcontext. The callback parameter is therefore namedcontext, while the native forward call correctly usesoutput_processor_context=....The hook's output-weight contract differs from the legacy copied forward:
output_weight;output_weight=None, and the callback usesoutput_layer.weight.The callback therefore resolves the fused-kernel weight with:
Under sequence parallelism, the callback gathers
hidden_statesbefore invokinglinear_cross_entropy, matching the established legacy behavior.Checklist Before Submitting
pre-commit run --all-filespassed.ci-requestchannel.recipesubmodule.AI assistance disclosure: OpenAI Codex assisted with implementation and test execution; I reviewed the resulting changes and test outputs.